Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WRR-387: Panels: Updated documentation for arranger #1752

Merged
merged 3 commits into from
Dec 19, 2024

Conversation

vJIYEv
Copy link
Contributor

@vJIYEv vJIYEv commented Nov 18, 2024

Checklist

  • I have read and understand the contribution guide
  • A CHANGELOG entry is included
  • At least one test case is included for this feature or bug fix
  • Documentation was added or is not needed
  • This is an API breaking change

Issue Resolved / Feature Added

Default value of arranger prop in Sandstone/Panels does not match the documentation.
Documentation says the default is ui/ViewManager.SlideLeftArranger, but actual value is BasicArranger from internals/Panles/Arrangers.js.

Resolution

Default value was changed in #315 from SlideLeftArranger to BasicArranger and arrange in BasicArranger was changed to deferArrange in #577
But BasicArranger is for internal use.
So instead of updating documentation of arranger in Sandstone/Panels, I updated documentation of BasicArranger to explain what it is.

Additional Considerations

Links

WRR-387

Comments

Enact-DCO-1.0-Signed-off-by: Jiye Kim (jiye.kim@lge.com)

Enact-DCO-1.0-Signed-off-by: Jiye Kim (jiye.kim@lge.com)
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.77%. Comparing base (3ad4630) to head (c3df2c8).
Report is 29 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1752      +/-   ##
===========================================
+ Coverage    80.80%   81.77%   +0.96%     
===========================================
  Files          148      148              
  Lines         6675     7022     +347     
  Branches      1985     2148     +163     
===========================================
+ Hits          5394     5742     +348     
- Misses         972      973       +1     
+ Partials       309      307       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MikyungKim MikyungKim changed the title WRR-387: Updated documentation for arranger WRR-387: 'Panels': Updated documentation for arranger Nov 20, 2024
@MikyungKim MikyungKim changed the title WRR-387: 'Panels': Updated documentation for arranger WRR-387: Panels: Updated documentation for arranger Nov 20, 2024
@@ -129,6 +129,9 @@ const deferArrange = (config, keyframes, options) => {
/**
* Arranger that slides panels in from the right and out to the left.
*
* This arranger is visually same as {@link ui/ViewManager.SlideLeftArranger} when transition but uses transform percentages instead of pixel values.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like ui/ViewManager.SlideLeftArranger already use percentages. So I think this comment should be revised.
If the visual is the same, what makes difference between SlideLeftArranger and BasicArranger?
I see BasicArranger uses deferArrange and offset. Please find more details about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. offset values here don't affect animation (https://developer.mozilla.org/en-US/docs/Web/API/Web_Animations_API/Keyframe_Formats)
  2. deferArrange was defined in PLAT-112222: Change BasicArranger to defer animation until idle #577 and it prevents skipping transition animations under system load. I updated the comments.

Enact-DCO-1.0-Signed-off-by: Jiye Kim (jiye.kim@lge.com)
Enact-DCO-1.0-Signed-off-by: Jiye Kim (jiye.kim@lge.com)
Copy link
Contributor

@MikyungKim MikyungKim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MikyungKim MikyungKim merged commit 7ec3f80 into develop Dec 19, 2024
7 checks passed
@MikyungKim MikyungKim deleted the feature/WRR-387 branch December 19, 2024 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants